Feed ErrorGuaranteed from early lifetime resolution errors through to bound variable resolution#152076
Feed ErrorGuaranteed from early lifetime resolution errors through to bound variable resolution#152076eggyal wants to merge 3 commits intorust-lang:mainfrom
ErrorGuaranteed from early lifetime resolution errors through to bound variable resolution#152076Conversation
|
Changes to the size of AST and/or HIR nodes. cc @nnethercote |
|
|
There was a problem hiding this comment.
Thanks for working on this! I won't be able to review this tonight but I'd like to note that thanks to your changes we should now be able to get rid of the minor hack in HIR ty lowering as I've mentioned in the issue:
rust/compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_trait.rs
Lines 454 to 464 in f60a0f1
(by replacing that whole snippet with just RegionInferReason::ObjectLifetimeDefault)
This comment was marked as resolved.
This comment was marked as resolved.
|
Preliminary sanity perf check due to size changes. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Bind undeclared lifetimes as erroneous bound vars
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e81e54f): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 3.1%, secondary -0.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 472.721s -> 472.345s (-0.08%) |
|
HIR ty lowering was modified cc @fmease |
| let span = *entry.get(); | ||
| let err = ResolutionError::NameAlreadyUsedInParameterList(ident, span); | ||
| self.report_error(param.ident.span, err); | ||
| let guar = self.r.report_error(param.ident.span, err); |
There was a problem hiding this comment.
This will unconditionally emit an error, whereas before it was suppressed in rustdoc; however I'm not sure that such suppression was correct here anyway—see the comments on Self::should_report_error, which don't appear to apply:
rust/compiler/rustc_resolve/src/late.rs
Lines 4718 to 4720 in a531436
In any event, this change doesn't appear to be breaking any tests...
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Right, this change is mildly concerning. I have to investigate if this is reachable, otherwise we risk regressing crates in the ecosystem.
This made me realize that rustdoc does already report some lifetime resolution failures in bodies, I did not know this. So while fn f() { undefined } gets accepted intentionally by rustdoc, fn f() { let _: &'a (); } does not. Interesting that nobody has complained so far ^^' (and I wouldn't wanna change it actually).
e7d86b7 to
a531436
Compare
There was a problem hiding this comment.
Whereas in 08ed1a1 (now fa5cccf) I was only feeding ErrorGuaranteed through to RBV in the specific case of undeclared named lifetimes, in a531436 (now 8e83a57) I've extended the logic to feed through such an ErrorGuaranteed for every early lifetime resolution error. This enables the aforementioned "hack" to be removed without giving rise to new instances of E0228 ("the lifetime bound for this object type cannot be deduced from context"), however it does also suppress a few other errors that previously were emitted—see updates to test outputs.
View changes since this review
I didn't squash (yet) in case you had begun reviewing and it was easier to see what has since changed. However, these commits really ought to be squashed (with commensurate updates to the PR description and commit comments) before merging, and review may be easier combined in any event.
ErrorGuaranteed from early lifetime resolution errors through to bound variable resolution
This comment has been minimized.
This comment has been minimized.
|
Reminder, once the PR becomes ready for a review, use |
Undeclared lifetimes that appear in user code always give rise to an error (E0261) early in name resolution. This patch adds such undeclared lifetimes to the relevant context's bound vars, so that they resolve to regions with `RegionKind::Error(ErrorGuaranteed)`, in order to suppress some ensuing diagnostics (that might be resolved if the lifetime becomes properly declared).
a531436 to
8e83a57
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
|
Size change was mitigated; rechecking perf. [@]bors try parent=035b01b794602d5861daa43ac792f372f8981ed7 |
This comment has been minimized.
This comment has been minimized.
Feed `ErrorGuaranteed` from early lifetime resolution errors through to bound variable resolution
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (2bf877f): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -0.6%, secondary 6.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.4%, secondary -3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 476.944s -> 476.24s (-0.15%) |
compiler/rustc_hir/src/hir.rs
Outdated
| Error(ErrorGuaranteed), | ||
| NotFound, |
There was a problem hiding this comment.
I'm super glad that we can obtain an ErrorGuaranteed in so many cases from the name resolver!
I'm not so happy about NotFound, we should try to fully get rid of it in favor of Error(ErrorGuaranteed). Moreover, in the current state I feel like its name (plus lack of docs) would probably raise questions among anybody stumbling upon it.
As you know, we only use this variant for cases where get_lifetime_res() yields None. However, in my eyes it's a bug if we don't register a resolution for some lifetimes since it most likely means we forgot to visit some parts of the crate. There's one exception: If the crate is already super busted in some ways and we've already reported errors because of that, it's fine if we fail to resolve something.
Locally, I replaced the unwrap_or(Error) with an unwrap(), ran all UI tests and only found 2 ICEs because of this change: tests/ui/lifetimes/issue-97193.rs and tests/ui/lifetimes/issue-97194.rs. Minimized: extern { fn f() { type T = &'a (); } }. This falls under the aforementioned exception: In the name resolver, we seem to skip items nested inside foreign fns which is reasonable, after all they shouldn't have a body in the first place (but then the AST lowerer does visit it thereby leading to the 'mismatch').
Long story short, I think we should dcx.delayed_bug(…) / dcx.span_delayed_bug(…) if get_lifetime_res rets None. This would give us the wanted ErrorGuaranteed.
There was a problem hiding this comment.
However, I've seen that you no longer (eagerly) register Error resolutions in two LifetimeRibKind::ElisionFailure cases. Does that lead to more cases where get_lifetime_res returns None or do we eventually register the Errors? I haven't really looked that closely. If it's the former, that would be unfortunate (I mean we could always add more delayed_bugs…).
There was a problem hiding this comment.
We still register the elision failures, they're just deferred until the error is emitted in resolve_fn_signature (as otherwise we don't have the ErrorGuaranteed to perform the registration); the error isn't emitted until then in order to collate all such elision failures on a function into a single diagnostic.
I totally agree with what you say about NotFound: I've removed it in 60ccf76 and, as suggested, am using a deferred bug instead. All tests are passing. Again, this will want squashing down in due course.
| let span = *entry.get(); | ||
| let err = ResolutionError::NameAlreadyUsedInParameterList(ident, span); | ||
| self.report_error(param.ident.span, err); | ||
| let guar = self.r.report_error(param.ident.span, err); |
There was a problem hiding this comment.
Right, this change is mildly concerning. I have to investigate if this is reachable, otherwise we risk regressing crates in the ecosystem.
This made me realize that rustdoc does already report some lifetime resolution failures in bodies, I did not know this. So while fn f() { undefined } gets accepted intentionally by rustdoc, fn f() { let _: &'a (); } does not. Interesting that nobody has complained so far ^^' (and I wouldn't wanna change it actually).
Undeclared lifetimes that appear in user code always give rise to an error (E0261) early in name resolution. This patch adds such undeclared lifetimes to the relevant context's bound vars, so that they resolve to regions with
RegionKind::Error(ErrorGuaranteed), in order to suppress some ensuing diagnostics (that might be resolved if the lifetime becomes properly declared).Fixes #152014
r? fmease